feat(core) Support multiple event managers (multi-canvas prep)#10375
feat(core) Support multiple event managers (multi-canvas prep)#10375ibgreen-openai wants to merge 2 commits into
Conversation
|
coverage: 83.442% (+0.02%) from 83.424% — ib/multi-canvas-event-manager-prep into master |
ibgreen-openai
requested review from
Pessimistress,
charlieforward9,
chrisgervang and
felixpalmer
ibgreen-openai
changed the title
ibgreen-openai
changed the title
ibgreen-openai
changed the title
ibgreen-openai
force-pushed
the
ib/multi-canvas-event-manager-prep
branch
from
85cee04 to
ff8fefb
Compare
| /** | ||
| * Get the event manager associated with a view or the default Deck canvas. | ||
| */ | ||
| getEventManager(viewId?: string): EventManager | null { |
There was a problem hiding this comment.
should console.warn. Fallback makes sense but should be a little less silent on a public method.
There was a problem hiding this comment.
should console.warn. Fallback makes sense but should be a little less silent on a public method.
While it would be nice to show developer friendly messages all over the API in cases like this, such message strings would all be adding to the bundle size of the application and since they have no value to end user, we generally avoid adding them unless value is very high.
| } | ||
|
|
||
| private _createEventManager(root: HTMLElement): EventManager { | ||
| const eventManager = new EventManager(root, { |
There was a problem hiding this comment.
@chrisgervang does it make sense to refactor my recent mobile tip+tricks work:
within cssProps here (rather than forcing user to workaround with external CSS)?
| | {[viewId: string]: AnyViewStateOf<ViewsT>}; | ||
|
|
||
| /** Canvas id used by views that do not declare a presentation canvas. */ | ||
| export const DEFAULT_CANVAS_ID = 'default-canvas'; |
There was a problem hiding this comment.
| /** | ||
| * Optional id of the presentation canvas associated with this view. | ||
| * Integrations that render views into multiple canvases can use this id to route view-scoped | ||
| * resources such as event managers. |
There was a problem hiding this comment.
| * resources such as event managers. | |
| * resources such as event managers. Default: `default-canvas` |
4 participants
Summary
Prepare the multi-canvas RFC by landing the view-scoped event-manager routing seam separately from the full rendering work.
Why
The follow-up
ib/multi-canvas-rfcbranch needs per-canvasEventManagerinstances, but the event-manager / Mjolnir.js-facing part is easier to review independently fromCanvasManager, presentation contexts, render/pick changes, and React multi-canvas DOM structure.This prep diff is behavior-preserving on
master: Deck still creates and uses the existing singletonEventManager, but Deck and ViewManager now route through a stable canvas-id seam so the RFC branch can later provide distinct managers per presentation canvas.What changed
canvasId?: stringview identity plumbing.ViewManager.getCanvasId(viewOrViewId)with a stabledefault-canvasfallback.EventManagercreation and stored it ineventManagersunder the default canvas id.Deck.getEventManager(viewId?: string); on this prep branch it still resolves the current singleton.eventManagersintoViewManagerand routed controller creation through each view's resolved canvas id/event manager binding.deck.getEventManager(viewId)seam.Intentionally not included
CanvasManager.DeckProps.canvases.Validation
yarn buildyarn test-headless test/modules/core/lib/view-manager.spec.ts test/modules/core/lib/deck.spec.tsyarn test-headless test/modules/react/utils/position-children-under-views.spec.ts test/modules/react/deckgl.spec.tsyarn lintyarn lintpasses with warnings only. The focused React test still emits the existingact(...)warning while passing.